Add kmCondBranch and kmCondCall hooks#59
Open
RoadrunnerWMC wants to merge 1 commit intoTreeki:masterfrom
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Status: Fully tested, ready to merge.
This PR adds new hook types
kmCondBranchandkmCondCall, which work as you'd expect:The conditional value is a 32-bit integer, as in
kmCondWrite32. This is expected to be an encoded PPC instruction.A concrete use case for this is the revamped Super Mario Galaxy / Super Mario Galaxy 2 Kamek loader I'm currently working on. I'm sure it'll be useful more broadly, too.
Kamekfile format and loader changes
To support this new feature, the loader is updated to use the new Kamekfile v3 format, which adds new
kCondBranchandkCondBranchLinkcommand types. No changes to game harnesses are required.It's worth pointing out that #54 also increments the Kamekfile version field, currently to 3. Whichever PR is merged second will have its version number changed to 4 instead.
"Def" versions
"Def" versions of these conditional calls can be easily added in a follow-up PR, if we want to.
Naming
This naming (
kmCondBranch,kmCondCall) is consistent with other Kamek hooks. However, it's worth pointing out that "conditional branch" is already an established phrase with a different meaning (e.g.beq), which is kind of unfortunate.Implementation notes
Rather than copypaste all the conditional-write code from the WriteCommand class to BranchCommand, which gets very hairy for certain output formats, I instead simplified the latter by making it delegate output generation to an equivalent WriteCommand when it can. This change fully passes my test suite (#52), so I believe it works correctly.
Discussion: Format of "original" value
This PR implements (as an example):
Rather than something like:
#54 adds the ability to write inline assembly injected into game-code addresses, and it might be tempting to adapt that technique to this case. However -- setting aside the fact that it'd be much more complicated to implement -- I don't think it's actually a good idea.
Potential benefits
Here are all of the benefits I can think of for being able to write inline assembly as in #54 (ignoring the "memory savings" benefit mentioned in that PR since it obviously doesn't apply here):
However, in this case, the "inline assembly" isn't actually custom code, but an instruction from the original game, which is a frozen, unchanging target. So here's how those arguments apply to this case:
To the example from earlier:
In fact, the first version above wouldn't even work, as I don't believe Kamek currently lets you define multiple writes to the same address, even if they're conditional. (Maybe that should be changed, but obviously that's out of scope here.) So the second version would be both shorter and would actually work as intended.
However, I believe the main use case for conditional writes is to make static patches that work with multiple game versions, such as Kamek loader harnesses. And static patches that work across game versions don't use address porting at all, so this benefit wouldn't apply. On the other hand, since it's not currently possible to mark writes in dynamic patches as only applying to specific game versions, conditional writes can still have uses in that mode.
All in all, this seems like an argument in favor of the inline-assembly syntax, although it's hard to say how much the hooks would actually be used this way in practice.
Syntax issues
Assembly instructions can have commas (e.g.
li r3, 0), which I imagine might confuse the preprocessor as it tries to figure out the number of arguments to the hypothetical inline-assembly versions of the macros:->
0x800db084{ li r30 }mySinFIdxThough I haven't tested it to see if it actually works that way.
Difficulty of implementation
As previously mentioned, this would be much more complicated to implement, which I don't think should be overlooked. It's not just a matter of "would it be better", but also "would it be worth adding that much more complexity to the linker, which would have to be maintained and supported forever."